Skip to content

Release fix memory leak#2571

Closed
iurisilvio wants to merge 17 commits into
Automattic:masterfrom
iurisilvio:release-fix-memory-leak
Closed

Release fix memory leak#2571
iurisilvio wants to merge 17 commits into
Automattic:masterfrom
iurisilvio:release-fix-memory-leak

Conversation

@iurisilvio
Copy link
Copy Markdown

Thanks for contributing!

  • Have you updated CHANGELOG.md?

The rotate90 and rotate270 lambdas in rotatePixels() allocated a
temporary 'unrotated' buffer via 'new uint8_t[n_bytes]' but never
freed it. Every JPEG decoded with EXIF orientation 90, 180, or 270
leaked the full pixel buffer (width * height * 4 bytes) — typically
several MiB per call.

Also corrects a 'new[]' / 'free()' allocator mismatch in
decodeJPEGIntoSurface's OOM error path (delete[] is the correct
deallocator for arrays allocated with new[]).
loadImage's onerror handler rejects the Promise but leaves image.src
pointing at the input buffer. When libjpeg/libpng allocated a cairo
surface before failing mid-decode, that surface stays attached to the
Image until V8 GC — under sustained load on malformed inputs this
delays cleanup arbitrarily.

Assign Buffer.alloc(0) before reject so clearData() runs synchronously.
If libjpeg calls error_exit during jpeg_read_scanlines (corrupt or
truncated JPEG that decoded the header but fails in the scan), it
longjmps back to the setjmp point in loadJPEGFromBuffer. That bypasses
the cleanup at the bottom of decodeJPEGIntoSurface, leaking both
'data' (width * height * 4) and 'src' (width * output_components).

Store both pointers on the canvas_jpeg_error_mgr so the error handler
can delete[] them before longjmp. Also reorder cleanup to destroy
libjpeg state and free src before creating the cairo surface, so the
success path is symmetric with the new failure path.
canvas_state_t held four cairo_pattern_t* members (fillPattern,
strokePattern, fillGradient, strokeGradient) but the destructor only
freed fontDescription. Every Context2d that ended life with a non-null
pattern leaked the pattern. The copy constructor copied the raw
pointers, so save()/restore() shared the same pattern across states
without refcounting — making any per-state destroy unsafe.

Switch to proper cairo refcount discipline:
- canvas_state_t copy ctor + new operator= reference each pattern.
- ~canvas_state_t releases all four patterns.
- New setFillPattern / setStrokePattern / setFillGradient /
  setStrokeGradient helpers replace the previous pattern (refcount--)
  before storing the new one (refcount++). clearFillPattern /
  clearStrokePattern release both pattern and gradient when switching
  to a solid color.

Also fix two correctness issues exposed by prompt finalization
(NAPI_EXPERIMENTAL path from PR Automattic#2562):
- _fillStyle and _strokeStyle now use Reset(obj, 1) (strong ref) so
  the JS gradient/pattern object stays alive while the Context2d's
  state still references its native cairo_pattern_t.
- ~Context2d pops all states before destroying _layout and _context
  so canvas_state_t destructors run with valid context.
- resetState clears all states (not just pop()) and re-points 'state'
  at the freshly emplaced top.
Bundles four additional fixes on top of v3.2.3-memfix.1:

- index.js loadImage clears image.src on error (defensive cleanup of
  partial cairo surfaces from malformed inputs).
- decodeJPEGIntoSurface frees data/src when libjpeg longjmps from
  jpeg_read_scanlines mid-decode (corrupt scan data).
- canvas_state_t takes refcounted ownership of cairo_pattern_t members
  (fillPattern, strokePattern, fillGradient, strokeGradient) so they
  don't leak when state is destroyed.
- _fillStyle/_strokeStyle hold strong refs to gradient/pattern JS
  objects so they can't be GC'd while in use under prompt finalization.
…unts

Don't call _resetPersistentHandles() from ~Context2d. napi_delete_reference
is unsafe inside GC and Node 22 panics with:

  FATAL ERROR: Finalizer is calling a function that may affect GC state.
  The finalizers are run directly from GC and must not affect GC state.

Persistent handles are already reset from Finalize(Napi::Env), which runs
deferred via node_api_post_finalizer when NAPI_EXPERIMENTAL is enabled.
@iurisilvio iurisilvio closed this May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant